Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

arch/ra4: Add support for Renesas RA4M1 MCU #15892

Merged
merged 3 commits into from
Feb 25, 2025
Merged

arch/ra4: Add support for Renesas RA4M1 MCU #15892

merged 3 commits into from
Feb 25, 2025

Conversation

leocafonso
Copy link
Contributor

Summary

Add basic support for RA4M1 and arduino-r4-minima. The following peripheral are added:

  • GPIO
  • SCI (UART)
  • Clock (Just internal clock - HOCO)

Impact

This update introduces support for the Renesas RA4M1 MCU, which powers the Arduino R4 Minima board. There is no impact on the existing NuttX codebase.

For more details, refer to the board documentation:

Documentation/platforms/arm/ra4m1/boards/arduino-r4-minima/index.rst

Testing

Successfully booted NSH on SCI2 (Arduino R4 Minima TX/RX).
Executed ostest successfully.

Final memory usage:
VARIABLE  BEFORE   AFTER
======== ======== ========
arena        6070     6070
ordblks         1        4
mxordblk     3500     1bb0
uordblks     2b70     3af8
fordblks     3500     2578
user_main: Exiting

@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Arch: arm Issues related to ARM (32-bit) architecture Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Feb 22, 2025
@leocafonso leocafonso marked this pull request as ready for review February 22, 2025 01:49
@nuttxpr
Copy link

nuttxpr commented Feb 22, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it's missing some details.

Strengths:

  • Clear Summary: The summary explains the "what" and "why" of the changes.
  • Impact Assessment: Addresses most impact areas. Specifically noting no impact on existing code is helpful.
  • Testing Evidence: Provides basic testing results, demonstrating functionality.
  • Documentation Reference: Points to relevant documentation.

Weaknesses (requiring clarification in the actual PR):

  • How the change works: The summary lacks detail on how these peripherals were added (new drivers, modifications to existing ones, etc.).
  • Related Issues: Missing links to any related NuttX issues or PRs. Even if none exist, explicitly stating that is helpful.
  • Build Host Details: Testing section lacks details on the build host environment. (OS, compiler version, etc.)
  • Complete "Before/After" Logs: While logs are provided, they are minimal. More comprehensive logs (including boot-up messages, potentially demonstrating peripheral usage) would strengthen the PR. The provided logs only show heap usage at the end of the ostest.
  • Target Details: "sim,RISC-V,ARM" is too generic. Specify the exact target architecture (e.g., armv7e-m). "board:config" needs to be more specific, like arduino-r4-minima:nsh.

Recommendation: The PR is on the right track, but needs minor improvements to be fully compliant with the NuttX requirements. Addressing the weaknesses listed above will significantly improve its reviewability and chances of acceptance.

acassis
acassis reviewed Feb 22, 2025
View reviewed changes
arch/arm/Kconfig Outdated
@@ -1155,6 +1160,7 @@ config ARCH_CHIP
default "nrf53" if ARCH_CHIP_NRF53
default "nrf91" if ARCH_CHIP_NRF91
default "nuc1xx" if ARCH_CHIP_NUC1XX
default "ra" if ARCH_CHIP_RA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leocafonso I suggest to use "ra4" because there are many ra architectures and families (RA0 [RA0E1], RA2 [RA2L1, RA2E1, RA2E2, RA2A1, RA4[...], ...). Keep all inside a single "ra" will result in a code with too many #ifdefs and that will make the code hard to read. I suggest to use same approach like stm32, stm32f7, stm32h7, etc.

Source: https://www.renesas.com/en/products/microcontrollers-microprocessors/ra-cortex-m-mcus

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acassis, I got your point. I have changed the arch to ra4. Thanks for the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this way, maybe later we could rename it to ra4m1 if we realize the internal peripherals are too different from a family to another

@acassis
Copy link
Contributor

acassis commented Feb 22, 2025

@leocafonso the CI also found these issues:

../nuttx/tools/checkpatch.sh -u -m -g fd6d804..HEAD
Error: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:2:1: error: Relative file path does not match actual file
Error: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:38:1: error: Too many blank lines
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:50:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:51:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:52:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:53:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:54:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:55:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:56:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:57:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:58:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:59:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:60:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:61:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:62:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:63:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:64:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:65:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:66:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:67:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:68:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:69:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:70:51: warning: Wrong column position of comment right of code
Warning: /home/runner/work/nuttx/nuttx/nuttx/arch/arm/include/ra/ra4m1_irq.h:71:51: warning: Wrong column position of comment right of code
Error: Process completed with exit code 1.

@leocafonso leocafonso changed the title arch/ra: Add support for Renesas RA4M1 MCU arch/ra4: Add support for Renesas RA4M1 MCU Feb 22, 2025
@leocafonso leocafonso force-pushed the feature/arduino-r4-minima branch 2 times, most recently from e7b7194 to eeeac31 Compare February 22, 2025 22:38
@acassis
Copy link
Contributor

acassis commented Feb 23, 2025

@leocafonso there is a last issue:

modified:   boards/arm/ra4/arduino-r4-minima/configs/nsh-leds/defconfig
modified:   boards/arm/ra4/arduino-r4-minima/configs/nsh/defconfig

These defconfig are out of sync, you can fix it this way:

$ ./tools/refresh.sh arduino-r4-minima:nsh-leds
$ ./tools/refresh.sh arduino-r4-minima:nsh

@leocafonso leocafonso force-pushed the feature/arduino-r4-minima branch 2 times, most recently from 4848784 to c4e638c Compare February 23, 2025 21:40
jerpelea
jerpelea requested changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please split the patch in several commits
documentation
arch
boards

@leocafonso
arch/ra4: Add support for Renesas RA4M1 MCU
cf643fa 
Add basic support for RA4M1.
The following perpheral are added:
* GPIO
* SCI (UART)
* Clock (Just internal clock - HOCO)

Signed-off-by: leocafonso <[email protected]>
@leocafonso
boards/ra4: Add support for Arduino-R4-Minima
a0d142c 
Add support for arduino-r4-minima.
Created defconfig and board source files for:
* nsh
* nsh-leds

Signed-off-by: leocafonso <[email protected]>
@leocafonso
Documentation/ra4m1: Add documentation for Renesas RA4M1 MCU and ardu…
d37162f 
…ino-r4-minima

Signed-off-by: leocafonso <[email protected]>
@leocafonso leocafonso force-pushed the feature/arduino-r4-minima branch from c4e638c to d37162f Compare February 25, 2025 01:11
@leocafonso leocafonso requested a review from jerpelea February 25, 2025 01:12
@acassis acassis merged commit 98b9cc2 into apache:master Feb 25, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acassis acassis acassis approved these changes

@lupyuen lupyuen lupyuen approved these changes

@adamfeuer adamfeuer Awaiting requested review from adamfeuer

@raiden00pl raiden00pl Awaiting requested review from raiden00pl

@xiaoxiang781216 xiaoxiang781216 Awaiting requested review from xiaoxiang781216

@Donny9 Donny9 Awaiting requested review from Donny9

@jerpelea jerpelea Awaiting requested review from jerpelea

Assignees
No one assigned
Labels
Arch: arm Issues related to ARM (32-bit) architecture Area: Documentation Improvements or additions to documentation Board: arm Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@leocafonso @nuttxpr @acassis @jerpelea @lupyuen